feat(phase-02a): packet 2 — shared kernel core - #6
Conversation
Land the SharedKernel primitives every later module depends on: deterministic-test clocks/random/guid factories (Standards 02 § Time), `LocalizedMessage` carrying the `lockey_` prefix invariant for `Result.Fail` payloads, refactored `Error` (wraps `LocalizedMessage`, projects `Code` over `Message.Key`), `Entity<TId>` + `AuditableEntity<TId>` aggregate bases with the audit-column / soft-delete / optimistic-concurrency contract, in-process `IDomainEvent : INotification`, cursor-first pagination matching Standards 04. Vogen 7.0.0 wired per ADR-0023 with `LearnStackVogenDefaults.IdMask`; `IAggregateRoot<TId>` constrains every aggregate to a strongly-typed ID. 54 unit tests + a Vogen emitter smoke test green. ADR: ADR-0023 Module: SharedKernel Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… 1 + event-outbox example
Two doc-only follow-ups surfaced by Phase 02a Packet 2 implementation:
- ADR-0023 Amendment 1 (2026-05-21): record the concrete Vogen 7.0.0 version
pin (6.0.7 was no longer on NuGet when Directory.Packages.props was wired),
and clarify that the `Aggregate_Roots_Use_StronglyTypedId` architecture
test lands with the first concrete aggregate (Packet 6 — Tenancy) rather
than Packet 2 (which has no aggregates yet — the test would be vacuous).
Decision section untouched.
- architecture/15 example: `LocalizedMessage.Of("enrollment.created")` did
not honour the `lockey_` prefix invariant Packet 2 just made canonical.
Updated to `LocalizedMessage.Of("lockey_enrollment_created")` and the
surrounding `Result.Success(...)` call switched to the canonical
`Result<EnrollmentDto>.Ok(...)` factory.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Blocker fixes from the dual review of db230ae: - Entity<TId> equality contract — transient entities (default Id) are now never equal to each other (only to themselves by reference), and a cross-runtime-type guard stops two distinct aggregate types that share the same Entity<TId> base + Id value from collapsing into the same change-tracker slot. - Result.FailFor<TResponse> shape — was `<T> => Result<T>.Fail`, returning Result<Result<TValue>> whenever the pipeline behavior's TResponse is Result<TValue>. Now constrained `where TResponse : IResultBase` and reflection-driven so it returns the concrete TResponse the handler signature expects. Non-Result generic argument fails loud. Major fixes: - Error.Code is now the unprefixed stable identifier ("validation_failed") projected from Message.Key by stripping the lockey_ prefix; the wire Problem Details code stays in sync with Standards 04 § Problem Details AND the lockey_ invariant by construction. Error.Details typed as IReadOnlyDictionary<string, IReadOnlyList<LocalizedMessage>> so field-level messages cannot bypass the invariant. - Result<T>.Ok throws on null value (Standards 09 § Forbidden); the primary constructor is now `internal` so positional record syntax cannot bypass the factory invariants. A new Unit value type covers the payload-less success shape. - DomainEvent.EventId / OccurredAt are `required init` — the BCL-default initializers bypassed IGuidFactory / IClock, defeating the deterministic rule the abstractions exist for. Concrete events now stamp via injected abstractions at the call site. - AuditableEntity actor columns (CreatedBy / UpdatedBy / DeletedBy) use a new strongly-typed UserId Vogen value object in SharedKernel.Identifiers rather than raw Guid; respects Standards 02 § Strongly-Typed Identifiers. The Identity module consumes the same UserId when it ships in 02b. ISoftDelete.DeletedBy stays Guid? at the marker layer (module-agnostic); AuditableEntity provides the Guid projection via explicit interface impl. - AuditableEntity.MarkCreated throws on second call (audit-trail integrity). SoftDelete also bumps UpdatedAt / UpdatedBy so "last touched" is monotonic; the audit pipeline still categorises the delete via DeletedAt. - CursorPagination ctor validates Limit > 0 (kernel-level guard); the API-layer FluentValidation is the place that translates malformed user input into Result.Fail. Normalised() no longer throws — only clamps. - SharedKernel build-time-only references documented in csproj comments: EF Core (Vogen-emitted converter requires it) and MediatR (IDomainEvent : INotification). The standards-side exception lands in the follow-up docs commit. Minors: - Files split per Standards 02 § File Organization: IHasId, IAggregateRoot, IResultBase, Result static helper, Unit, LocalizedMessage all live in their own file. Entity<TId>.DomainEvents returns the backing list directly (no per-call AsReadOnly allocation). - LocalizedMessage defensively copies Params at construction so caller mutations cannot leak in; structural equality (Key + Params) implemented explicitly so two messages built from the same source still compare equal. Empty params dictionary normalised to null. XSS contract documented (Params values must be plain text — frontend resolves via React text nodes, never dangerouslySetInnerHTML). - ISoftDelete.IsDeleted DIM removed; AuditableEntity exposes IsDeleted directly so the contract is consistent per access path. - FixedGuidFactory shared-queue contract documented in the XML doc. Tests: 64 unit + 17 architecture + 1 contract green. New coverage for transient + cross-type equality, FailFor<Result<T>>, Ok null guard, MarkCreated second-call throw, CursorPagination ctor validation, and LocalizedMessage defensive copy. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Records the corpus-side decisions that the code-side fixes (commit 7c9133a) needed to land cleanly. Pure documentation; no code change. - Standards 01 § Dependency Direction grows a "Build-time-only exceptions" sub-section spelling out the two sanctioned SharedKernel external references (Microsoft.EntityFrameworkCore via the Vogen-emitted EfCoreValueConverter, MediatR via IDomainEvent : INotification) with the ADR they trace to. Adding a third such reference now explicitly requires a new ADR. Names the follow-up architecture test that encodes the rule alongside the first Module.Domain aggregate in Packet 6. - ADR-0023 Implementation Notes literal pin: the placeholder Version="..." is replaced with the concrete Version="7.0.0" already recorded in Amendment 1; the SharedKernel cross-cutting value-object list gains UserId; the EF Core transitive-reference rule is cross-linked to Standards 01. - Standards 02 + 09 Result Type snippets reflect the refactored shape: internal primary constructor, Ok throws on null value, Error.Details typed as IReadOnlyDictionary<string, IReadOnlyList<LocalizedMessage>>, Error.Code projects from Message.Key by stripping the lockey_ prefix, Result<Unit> is the canonical payload-less success. The Standards 09 error-code table prose is updated to the "table omits the prefix; wire format is the prefixed key, Code is the unprefixed stable identifier" shape that satisfies both Standards 04 § Problem Details and Standards 09 § Forbidden. - Glossary entries refreshed: Result<T> (internal ctor, Unit), Error (Code stripping, Details with LocalizedMessage lists), LocalizedMessage (structural equality, defensive copy, plain-text param contract), Entity / AuditableEntity (transient + cross-type equality guards, MarkCreated throws, SoftDelete bumps UpdatedAt), IDomainEvent (required init), CursorPagination (ctor guard); new UserId and Unit entries. - Phase 02a Packet 2 status block records the review pass folded in, pointing to commit 7c9133a. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reviewer's GuideIntroduces the SharedKernel core primitives (time/random/guid abstractions, localized messaging, result/error model, entities/auditing, domain events, pagination, strongly-typed IDs) plus associated tests and documentation updates, forming the base contracts later modules depend on. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR implements Phase 02a Packet 2 of the shared-kernel foundation: domain entity bases with audit and domain-events, Vogen-backed strongly-typed IDs and factories, localized Result/Error types and Unit, cursor pagination primitives, deterministic test abstractions (IClock/IRandom/IGuidFactory), and many unit tests and docs updates. ChangesShared Kernel Core Infrastructure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="backend/tests/LearnStack.Tests.Unit/SharedKernel/Identifiers/VogenIdEmissionTests.cs" line_range="23-32" />
<code_context>
+ private static readonly UserId Actor =
+ UserId.From(Guid.Parse("019712ac-aaaa-7000-8000-000000000aaa"));
+
+ [Fact]
+ public void MarkCreated_SetsCreatedAtAndCreatedBy()
+ {
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a TypeConverter round-trip test for Vogen-emitted IDs
The doc comment on `VogenIdEmissionTests` lists TypeConverter support as one of the four artifacts `TestId` should validate, but the tests only cover `IStronglyTypedId.Value`, `System.Text.Json` round-trip, and equality. To complete that coverage, please add a TypeConverter round-trip test like the example above so the `LearnStackVogenDefaults.IdMask` contract and TypeConverter-based scenarios (e.g., route binding/configuration) are exercised by tests.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Code Review
This pull request establishes the core SharedKernel foundation, introducing base aggregate classes (Entity and AuditableEntity), domain event infrastructure, and deterministic abstractions for time, randomness, and GUID generation. It implements a standardized result pattern using Result and LocalizedMessage to enforce localization invariants, alongside cursor-based pagination models and strongly-typed identifiers via Vogen. Feedback suggested optimizing the reflection-based Result.FailFor helper by caching MethodInfo to prevent performance bottlenecks and refining the Unit struct implementation to ensure the singleton instance is explicitly initialized.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
backend/tests/LearnStack.Tests.Unit/SharedKernel/Identifiers/VogenIdEmissionTests.cs (1)
9-20: ⚡ Quick winTypeConverter coverage claim does not match implemented tests.
Lines 14-17 state this fixture verifies TypeConverter parse/format, but no test currently exercises that path. Please either add the missing test or trim the XML summary to match actual coverage.
Possible test addition
+using System.ComponentModel; using System.Text.Json; ... [Fact] + public void TypeConverter_RoundTrip_PreservesValue() + { + var original = TestId.New(); + var converter = TypeDescriptor.GetConverter(typeof(TestId)); + + var asString = converter.ConvertToInvariantString(original); + var parsed = converter.ConvertFromInvariantString(asString); + + parsed.Should().Be(original); + } + + [Fact] public void TwoIdsWithDifferentGuids_AreNotEqual() { TestId.New().Should().NotBe(TestId.New()); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/tests/LearnStack.Tests.Unit/SharedKernel/Identifiers/VogenIdEmissionTests.cs` around lines 9 - 20, The XML summary claims Conversions.TypeConverter coverage but no test exercises it; add a unit test in the VogenIdEmissionTests class (e.g., TypeConverter_ParseAndFormat_RoundTrips) that uses TypeDescriptor.GetConverter(typeof(TestId)) to obtain the converter and asserts ConvertFrom/ConvertTo round-trips a TestId (or a Guid/string) and that parsing/formatting yields the expected TestId.Value; reference TestId, VogenIdEmissionTests, TypeDescriptor.GetConverter, ConvertFrom and ConvertTo when implementing the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/src/LearnStack.SharedKernel/Domain/AuditableEntity.cs`:
- Around line 64-101: The MarkCreated, MarkUpdated, and SoftDelete methods
currently accept default DateTimeOffset and UserId values and may persist
invalid audit metadata; update each method to validate inputs before assigning
audit fields: ensure the DateTimeOffset parameter 'at' is not the default value
and the UserId parameter 'by' is not null/its default, and throw an
ArgumentException (or ArgumentNullException) with a clear message if validation
fails; keep the existing immutability check in MarkCreated for
CreatedAt/CreatedBy and then assign CreatedAt/CreatedBy (MarkCreated),
UpdatedAt/UpdatedBy (MarkUpdated), and DeletedAt/DeletedBy plus
UpdatedAt/UpdatedBy (SoftDelete) only after validation.
In `@backend/src/LearnStack.SharedKernel/Domain/Entity.cs`:
- Line 51: The DomainEvents property currently returns the mutable backing list
(_domainEvents) directly; change it to return an immutable/read-only wrapper so
callers cannot cast and mutate it. Update the DomainEvents getter in Entity<TId>
to return a read-only collection (e.g. _domainEvents.AsReadOnly() or new
ReadOnlyCollection<IDomainEvent>(_domainEvents) or _domainEvents.ToArray())
instead of returning _domainEvents directly, ensuring the field _domainEvents
remains private and only mutated via aggregate methods.
In `@backend/src/LearnStack.SharedKernel/Localization/LocalizedMessage.cs`:
- Around line 50-52: Params is being assigned a mutable Dictionary which allows
external mutation and breaks immutability/equality; update the assignment in the
LocalizedMessage constructor so Params is an immutable/read-only collection
(e.g., change the stored type to IReadOnlyDictionary<string,string> or wrap the
incoming Dictionary with a read-only wrapper or create an ImmutableDictionary
via ImmutableDictionary.CreateRange(`@params`)) and assign that instead of new
Dictionary<string,string>(`@params`) so callers cannot mutate Params after
construction.
In `@backend/src/LearnStack.SharedKernel/Persistence/ISoftDelete.cs`:
- Line 14: The ISoftDelete interface currently exposes a primitive Guid? via the
DeletedBy property; change DeletedBy to use the strongly-typed identifier (e.g.,
UserId?) instead to enforce type-safety across modules: update the ISoftDelete
interface declaration (DeletedBy) to UserId?, update any implementing classes
and consumers to accept/return UserId, and adjust mapping/EF Core configuration
and serialization code that persisted or read DeletedBy to handle the new UserId
type (including conversions or value object mapping if necessary) so no
primitive Guid usage remains for audit identity.
In `@backend/src/LearnStack.SharedKernel/Time/FixedClock.cs`:
- Around line 11-20: The FixedClock constructor, SetUtcNow, and Advance store
DateTimeOffset values with arbitrary offsets which can violate the UtcNow
contract; normalize all incoming/stored instants to UTC by calling
ToUniversalTime() (or ToOffset(TimeSpan.Zero)) before assigning to the backing
field _utcNow. Update the constructor (FixedClock(DateTimeOffset utcNow)),
SetUtcNow(DateTimeOffset utcNow), and Advance(TimeSpan delta) to assign _utcNow
= utcNow.ToUniversalTime() and _utcNow = _utcNow.Add(delta).ToUniversalTime()
respectively so UtcNow always exposes a UTC-offset DateTimeOffset.
---
Nitpick comments:
In
`@backend/tests/LearnStack.Tests.Unit/SharedKernel/Identifiers/VogenIdEmissionTests.cs`:
- Around line 9-20: The XML summary claims Conversions.TypeConverter coverage
but no test exercises it; add a unit test in the VogenIdEmissionTests class
(e.g., TypeConverter_ParseAndFormat_RoundTrips) that uses
TypeDescriptor.GetConverter(typeof(TestId)) to obtain the converter and asserts
ConvertFrom/ConvertTo round-trips a TestId (or a Guid/string) and that
parsing/formatting yields the expected TestId.Value; reference TestId,
VogenIdEmissionTests, TypeDescriptor.GetConverter, ConvertFrom and ConvertTo
when implementing the test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8dd919f4-0954-400d-8d37-df1c7e3495a9
📒 Files selected for processing (55)
backend/Directory.Packages.propsbackend/src/LearnStack.SharedKernel/Domain/AuditableEntity.csbackend/src/LearnStack.SharedKernel/Domain/DomainEvent.csbackend/src/LearnStack.SharedKernel/Domain/Entity.csbackend/src/LearnStack.SharedKernel/Domain/IDomainEvent.csbackend/src/LearnStack.SharedKernel/Domain/IHasDomainEvents.csbackend/src/LearnStack.SharedKernel/Identifiers/FixedGuidFactory.csbackend/src/LearnStack.SharedKernel/Identifiers/IAggregateRoot.csbackend/src/LearnStack.SharedKernel/Identifiers/IGuidFactory.csbackend/src/LearnStack.SharedKernel/Identifiers/IHasId.csbackend/src/LearnStack.SharedKernel/Identifiers/LearnStackVogenDefaults.csbackend/src/LearnStack.SharedKernel/Identifiers/SystemGuidFactory.csbackend/src/LearnStack.SharedKernel/Identifiers/UserId.csbackend/src/LearnStack.SharedKernel/LearnStack.SharedKernel.csprojbackend/src/LearnStack.SharedKernel/Localization/LocalizedMessage.csbackend/src/LearnStack.SharedKernel/Pagination/CursorPagination.csbackend/src/LearnStack.SharedKernel/Pagination/Page.csbackend/src/LearnStack.SharedKernel/Pagination/PageInfo.csbackend/src/LearnStack.SharedKernel/Persistence/IOptimisticConcurrency.csbackend/src/LearnStack.SharedKernel/Persistence/ISoftDelete.csbackend/src/LearnStack.SharedKernel/Random/FixedRandom.csbackend/src/LearnStack.SharedKernel/Random/IRandom.csbackend/src/LearnStack.SharedKernel/Random/SystemRandom.csbackend/src/LearnStack.SharedKernel/Results/Error.csbackend/src/LearnStack.SharedKernel/Results/IResultBase.csbackend/src/LearnStack.SharedKernel/Results/Result.Helpers.csbackend/src/LearnStack.SharedKernel/Results/Result.csbackend/src/LearnStack.SharedKernel/Results/Unit.csbackend/src/LearnStack.SharedKernel/Time/FixedClock.csbackend/src/LearnStack.SharedKernel/Time/IClock.csbackend/src/LearnStack.SharedKernel/Time/SystemClock.csbackend/tests/LearnStack.Tests.Unit/LearnStack.Tests.Unit.csprojbackend/tests/LearnStack.Tests.Unit/SharedKernel/Domain/AuditableEntityTests.csbackend/tests/LearnStack.Tests.Unit/SharedKernel/Domain/DomainEventTests.csbackend/tests/LearnStack.Tests.Unit/SharedKernel/Domain/EntityTests.csbackend/tests/LearnStack.Tests.Unit/SharedKernel/Domain/TestAggregate.csbackend/tests/LearnStack.Tests.Unit/SharedKernel/Domain/TestId.csbackend/tests/LearnStack.Tests.Unit/SharedKernel/ErrorTests.csbackend/tests/LearnStack.Tests.Unit/SharedKernel/Identifiers/FixedGuidFactoryTests.csbackend/tests/LearnStack.Tests.Unit/SharedKernel/Identifiers/SystemGuidFactoryTests.csbackend/tests/LearnStack.Tests.Unit/SharedKernel/Identifiers/VogenIdEmissionTests.csbackend/tests/LearnStack.Tests.Unit/SharedKernel/Localization/LocalizedMessageTests.csbackend/tests/LearnStack.Tests.Unit/SharedKernel/Pagination/CursorPaginationTests.csbackend/tests/LearnStack.Tests.Unit/SharedKernel/Pagination/PageTests.csbackend/tests/LearnStack.Tests.Unit/SharedKernel/Random/FixedRandomTests.csbackend/tests/LearnStack.Tests.Unit/SharedKernel/ResultTests.csbackend/tests/LearnStack.Tests.Unit/SharedKernel/Time/FixedClockTests.csbackend/tests/LearnStack.Tests.Unit/SharedKernel/Time/SystemClockTests.csdocs/architecture/15-event-and-outbox.mddocs/decisions/0023-strongly-typed-id-source-generator.mddocs/glossary.mddocs/roadmap/phase-02a-kernel-tenancy.mddocs/standards/01-architecture-standards.mddocs/standards/02-backend-coding.mddocs/standards/09-error-handling.md
…-safe collections All seven findings from the second review pass verified against current code and applied. No skips. Inline findings: - AuditableEntity.MarkCreated / MarkUpdated / SoftDelete now reject default(DateTimeOffset) and the Guid.Empty UserId via a shared EnsureValidAuditInput helper - default sentinels are programmer errors, never legitimate audit metadata. - Entity<TId>.DomainEvents returns a cached ReadOnlyCollection<T> wrapper initialised once per entity rather than the backing List<T> directly, so callers cannot downcast and mutate the collection out from under the aggregate. Wrapper is a one-time reference allocation; hot-path access stays allocation-free. - LocalizedMessage.Params stores a ReadOnlyDictionary wrapper around the defensive copy of the input dictionary - the IReadOnlyDictionary slot no longer allows a downcast back to Dictionary. - ISoftDelete.DeletedBy is UserId? (Standards 02 - no raw Guid on the marker surface). The explicit-interface-impl shim in AuditableEntity goes away; EF's Vogen value converter handles the column mapping. - FixedClock normalises every stored instant to UTC via ToUniversalTime() at the boundary so IClock.UtcNow always honours its contract regardless of the caller's input offset. Nitpicks: - VogenIdEmissionTests now exercises TypeConverter (the artefact ASP.NET Core minimal-API route binding and IConfiguration use) via TypeDescriptor.GetConverter round-trip - closes the coverage gap the fixture's doc summary advertised. - Result.FailFor<TResponse> caches the resolved MethodInfo in a nested FailForCache<TResponse> generic class so the reflection cost is paid once per closed Result<T> type, not on every pipeline invocation. Drops the redundant MakeGenericType reconstruction (TResponse is already the closed generic). - Unit.Value is now an expression-bodied `=> default` rather than an uninitialised auto-property; idiomatic and removes the dead backing field. Tests: 71 unit (+7) green, 17 architecture green, 1 contract green under CI=true. New coverage: audit-input default-rejection (timestamp + empty actor), FixedClock non-UTC offset normalisation, ISoftDelete.DeletedBy strong typing, TypeConverter round-trip. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…e wiring + doc sync
All 13 findings from the third review pass (one APPROVE with 3 minor + 4
suggestion, one REQUEST CHANGES with 5 major + 2 minor + 1 suggestion)
verified against current code and applied.
Code (kernel) - majors:
- CursorPagination ctor now clamps `Limit > MaxLimit` to `MaxLimit`
silently (still throws on `Limit <= 0` as a programmer-error guard).
Removed `Normalised()` - the invariant is enforced at construction so
no instance can exist with an out-of-range `Limit`. Drops the DoS
footgun where a caller forgot to normalise and passed `int.MaxValue`
into `.Take(...)`.
- Error switched to an explicit constructor: `ThrowIfNull(Message)` plus
defensive snapshot of `Details` (dictionary copy + per-key
`ReadOnlyCollection<LocalizedMessage>` wrap) so callers cannot mutate
the validation map after the Result is in flight - Problem Details,
audit, and log sinks all read after handler return. Structural Equals
+ GetHashCode override because `IReadOnlyDictionary` defaults to
reference equality.
- UserId.New() removed - the convenience static was a tempting bypass of
`IGuidFactory`. New UserIds in handlers now require
`UserId.From(guidFactory.NewUuidV7())` at the call site.
Code (kernel) - minors:
- Entity<TId>.DomainEvents lazily allocates both the backing List<T> and
the cached ReadOnlyCollection wrapper on first raise / first read. EF
Core's read path materialises every loaded aggregate via the
parameterless ctor; the lazy approach keeps materialisation
allocation-free for the common query case (paginated reads,
projections) and pays the allocation only when an aggregate actually
raises events (command paths).
- LearnStackVogenDefaults moved from `LearnStack.SharedKernel.Identifiers`
to the root `LearnStack.SharedKernel` namespace - the mask covers
aggregate IDs AND richer value objects per ADR-0023, so neither
surface owns the constant exclusively. The Vogen attribute on UserId
/ TestId still resolves it via parent-namespace lookup; TestId in the
test project gains an explicit `using LearnStack.SharedKernel;`.
- AuditableEntity.IsDeleted XML doc is now honest about EF translation:
the property is a computed CLR get and is NOT guaranteed to translate
by EF Core's expression translator. Global query filters in Packet 7
should gate on `e.DeletedAt == null` directly (the mapped column
translates cleanly).
Tests:
- CursorPaginationTests rewritten: ctor clamp, boundary cases
(Limit == MaxLimit, Limit == 1), removed all `Normalised()` references.
- ErrorTests: ctor null-Message throw, defensive-copy assertion (caller
mutates the source dictionary + list after construction; the Error's
Details remain stable), structural-equality across distinct dictionary
instances.
- FixedGuidFactoryTests: mixed V7+V4 seed test locks the documented
shared-queue contract.
- Total: 77 unit (+6 since previous round) + 17 architecture + 1
contract green under CI=true.
Module wiring (B.M5):
- New `backend/src/Modules/Directory.Build.props` adds the Vogen +
Microsoft.EntityFrameworkCore PackageReferences to every
`Modules.<X>.Domain` project via `EndsWith('.Domain')` condition.
Explicitly imports the parent `backend/Directory.Build.props` because
MSBuild stops at the first ancestor Directory.Build.props it finds
rather than merging up. This closes the gap ADR-0023 § Implementation
Notes promised but the original Packet 2 land did not deliver:
`[ValueObject<Guid>(LearnStackVogenDefaults.IdMask)]` on a
TenantId / CourseId / OrganizationId etc. in Packet 6+ will now
"just work" without per-csproj wiring.
Docs:
- Standards 02 § Strongly-Typed Identifiers snippet rewritten to the
canonical Vogen form (the stale hand-rolled example with
`Guid.NewGuid()` was misleading new module authors). Adds the
no-`New()` rule: IDs mint via `IGuidFactory` at the call site.
- Standards 09 § API Surface example refreshed: top-level uses
`messageKey` (`lockey_*`) and `code` (unprefixed stable id); field
errors flow as LocalizedMessage payloads (`key` + optional `params`)
so the lockey_ invariant covers the entire wire shape, not just the
top-level message.
- architecture/15 handler example: `OccurredAt = _clock.UtcNow` instead
of `DateTime.UtcNow` (closes the symmetric `IClock`-bypass anti-
example the reviewer caught).
- Phase 02a Packet 2 roadmap status: test counts refreshed (54 / 64 →
71 — actually now 77 after this round); TypeConverter coverage noted.
Suggestion not adopted:
- IRandom rename / NextBytes removal — deferred to the Packet 3+
analyzer pass (both reviewers raised it as a future-analyzer
concern). The XML doc already warns "not for crypto"; an analyzer
banning IRandom in *.Security.* / *.Auth.* namespaces is the right
enforcement surface and lands with the Roslyn analyzer infrastructure.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/src/LearnStack.SharedKernel/Pagination/CursorPagination.cs`:
- Around line 40-42: CursorPagination's public init on Limit allows callers to
bypass the constructor invariant (e.g., new CursorPagination { Limit = 0 } or
with { Limit = 0 }); change the Limit accessor to prevent external init
assignment (e.g., make the init accessor non-public such as private init or use
a private setter) so only the constructors/factory methods can set and validate
the value, keep the existing constructor checks in CursorPagination to enforce
1..MaxLimit, and if you still need a safe "with"-style mutation provide a
validated factory/WithValidatedLimit method that enforces the same range.
In `@backend/src/LearnStack.SharedKernel/Results/Error.cs`:
- Around line 122-127: The snapshot creation in SnapshotDetails copies each list
without validating individual LocalizedMessage entries, allowing null elements
to leak in and later crash in GetHashCode or DetailsEqual; update
SnapshotDetails to iterate each list (referencing variable names source, key,
list) and validate every element is non-null (throw ArgumentNullException or
ArgumentException that includes the key/context) before constructing the
ReadOnlyCollection<LocalizedMessage>, so the method fails fast on null entries.
In
`@backend/tests/LearnStack.Tests.Unit/SharedKernel/Pagination/CursorPaginationTests.cs`:
- Around line 59-66: The existing test Ctor_CarriesCursor only verifies creating
CursorPagination via its constructor; add a regression test that also constructs
the object via an object initializer and via a 'with' expression to ensure the
CursorPagination type preserves Cursor and Limit values (and that Limit cannot
be changed unexpectedly). Specifically, create a new test method (e.g.,
CtorAndInit_CarriesCursorAndLimit) that builds one instance with new
CursorPagination { Cursor = "abc", Limit = 50 } and another via the record
'with' syntax from an existing instance, then assert both Cursor and Limit equal
the expected values using the same assertion style as Ctor_CarriesCursor so
future reintroduction of mutability will be caught.
In `@docs/roadmap/phase-02a-kernel-tenancy.md`:
- Line 54: Update the three occurrences of the phrase "71 unit tests" in the
roadmap document to match the actual CI-reported test counts; replace them with
the reconciled numbers (e.g., "unit 77, architecture 17, contract 1") or
otherwise rephrase to reference CI test counts, ensuring the exact string "71
unit tests" is removed and the document consistently reflects the PR/CI summary.
In `@docs/standards/02-backend-coding.md`:
- Line 66: Add a proper "Derives from: ADR-0031" header to
docs/standards/02-backend-coding.md and convert the inline plain-text reference
"per ADR-0031" into an explicit link pointing to
docs/decisions/0031-postgresql-major-version.md so readers can jump to the ADR
that documents gen_uuid_v7(); ensure the linked text is exactly "ADR-0031" and
place the Derives from header near the top of the standards file consistent with
other standards documents.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d51d4c6a-bbab-4653-b85d-d96f64214fca
📒 Files selected for processing (23)
backend/src/LearnStack.SharedKernel/Domain/AuditableEntity.csbackend/src/LearnStack.SharedKernel/Domain/Entity.csbackend/src/LearnStack.SharedKernel/Identifiers/UserId.csbackend/src/LearnStack.SharedKernel/LearnStackVogenDefaults.csbackend/src/LearnStack.SharedKernel/Localization/LocalizedMessage.csbackend/src/LearnStack.SharedKernel/Pagination/CursorPagination.csbackend/src/LearnStack.SharedKernel/Persistence/ISoftDelete.csbackend/src/LearnStack.SharedKernel/Results/Error.csbackend/src/LearnStack.SharedKernel/Results/Result.Helpers.csbackend/src/LearnStack.SharedKernel/Results/Unit.csbackend/src/LearnStack.SharedKernel/Time/FixedClock.csbackend/src/Modules/Directory.Build.propsbackend/tests/LearnStack.Tests.Unit/SharedKernel/Domain/AuditableEntityTests.csbackend/tests/LearnStack.Tests.Unit/SharedKernel/Domain/TestId.csbackend/tests/LearnStack.Tests.Unit/SharedKernel/ErrorTests.csbackend/tests/LearnStack.Tests.Unit/SharedKernel/Identifiers/FixedGuidFactoryTests.csbackend/tests/LearnStack.Tests.Unit/SharedKernel/Identifiers/VogenIdEmissionTests.csbackend/tests/LearnStack.Tests.Unit/SharedKernel/Pagination/CursorPaginationTests.csbackend/tests/LearnStack.Tests.Unit/SharedKernel/Time/FixedClockTests.csdocs/architecture/15-event-and-outbox.mddocs/roadmap/phase-02a-kernel-tenancy.mddocs/standards/02-backend-coding.mddocs/standards/09-error-handling.md
…element null check
All 5 findings from the fourth review pass verified valid against current
code (no skips).
- CursorPagination.Limit invariant moved from the constructor body into
the property's `init` accessor. The previous shape let callers bypass
the guard via object-initializer syntax (`new CursorPagination { Limit = 0 }`)
and the record's `with` expression (`request with { Limit = 0 }`),
because both run AFTER the constructor and write to the public init
setter directly. Validating in `init` covers every initialisation
path with a single chokepoint. Backing field stays private; the
ctor delegates to the property.
- Error.SnapshotDetails null-checks each LocalizedMessage element, not
just the list reference. A null entry inside a per-field list would
later NPE in `GetHashCode` (`msg.GetHashCode()`) or `DetailsEqual`
(`listA[i].Equals(listB[i])`); failing fast at construction with the
offending key + index in the ArgumentException message gives the
caller the locator instead of a downstream NullReferenceException.
- CursorPaginationTests gains regression coverage for the init paths:
- `ObjectInitializer_AndWithExpression_PreserveValuesAndInvariants`
constructs via object initializer and `with`, asserts both shape
invariants hold.
- `ObjectInitializer_ZeroLimit_Throws` and `WithExpression_ZeroLimit_Throws`
lock the guard on both init paths.
- `ObjectInitializer_AboveMaxLimit_Clamps` verifies the clamp also
fires through the init accessor, not just the ctor.
- Phase 02a roadmap: removed the stale literal "71 unit tests" mentions
(two occurrences). The roadmap now references "Unit / architecture /
contract suites all green in CI" instead of pinning a number that
drifts with every review-driven test addition. CI summary is the
authoritative count.
- Standards 02 § Derives from now lists ADR-0031 (PostgreSQL major
version) alongside ADR-0002 / 0006 / 0023; the inline `gen_uuid_v7()`
reference is a proper markdown link to the ADR rather than plain text.
Tests: 81 unit (+4) + 17 architecture + 1 contract green under CI=true.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/standards/02-backend-coding.md (1)
39-39:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTypes-table example contradicts the new Vogen canonical declaration.
Line 39 still documents
record struct CourseId(Guid Value) : IStronglyTypedIdwith aGuid Valueconstructor parameter, but the Strongly-Typed Identifiers section (lines 49-52) now mandates the Vogen-annotatedpartial record struct CourseId : IStronglyTypedId<Guid>;shape with no positionalValueparameter. Readers skimming the Types table will copy the wrong form.📝 Proposed fix
-- **Strongly-typed ids** (`record struct CourseId(Guid Value) : IStronglyTypedId`) for all entity identifiers. Never expose raw `Guid` on the public surface. +- **Strongly-typed ids** for all entity identifiers — declared via Vogen as `[ValueObject<Guid>(...)] partial record struct CourseId : IStronglyTypedId<Guid>;` (see [§ Strongly-Typed Identifiers](`#strongly-typed-identifiers`)). Never expose raw `Guid` on the public surface.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/standards/02-backend-coding.md` at line 39, Update the Types table entry so it matches the Vogen canonical declaration used elsewhere: replace the old positional form `record struct CourseId(Guid Value) : IStronglyTypedId` with the Vogen-style shape `partial record struct CourseId : IStronglyTypedId<Guid>;` (i.e., no positional Value parameter and using partial record struct) so the CourseId example aligns with the Strongly-Typed Identifiers section and prevents copy-paste errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@docs/standards/02-backend-coding.md`:
- Line 39: Update the Types table entry so it matches the Vogen canonical
declaration used elsewhere: replace the old positional form `record struct
CourseId(Guid Value) : IStronglyTypedId` with the Vogen-style shape `partial
record struct CourseId : IStronglyTypedId<Guid>;` (i.e., no positional Value
parameter and using partial record struct) so the CourseId example aligns with
the Strongly-Typed Identifiers section and prevents copy-paste errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 02374f25-2bf1-4540-8370-810e03b9af52
📒 Files selected for processing (5)
backend/src/LearnStack.SharedKernel/Pagination/CursorPagination.csbackend/src/LearnStack.SharedKernel/Results/Error.csbackend/tests/LearnStack.Tests.Unit/SharedKernel/Pagination/CursorPaginationTests.csdocs/roadmap/phase-02a-kernel-tenancy.mddocs/standards/02-backend-coding.md
…le aligns with Vogen pattern Finding verified valid against current code: standards/02 line 39 (Types table) still showed `record struct CourseId(Guid Value) : IStronglyTypedId`, the pre-Vogen positional form, while § Strongly-Typed Identifiers immediately below already uses the Vogen canonical shape (`[ValueObject<Guid>(LearnStackVogenDefaults.IdMask)] partial record struct`). A reader scanning the Types table would copy the wrong form. Replaced the bullet's inline example with `partial record struct CourseId : IStronglyTypedId<Guid>;` and added a same-doc anchor link to the detailed § Strongly-Typed Identifiers section so readers land on the full Vogen pattern (attribute + From() factory + IGuidFactory rule) without ambiguity. Doc-only change; no build/test impact. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
Phase 02a Packet 2 — the SharedKernel primitives every later module inherits from. Four commits, structured as land + corrective ADR + post-review fix + corpus sync.
What ships
IClock/IRandom/IGuidFactorytriad +System*andFixed*counterparts (Standards 02 § Time).LocalizedMessagecarrying thelockey_prefix invariant at the constructor — every user-facing string the API ships now flows as a typed key, never raw English.Result<T>+Errorrefactor:Error.Codeis the unprefixed stable identifier projected fromMessage.Key(Standards 04 § Problem Details),Error.DetailsflowsLocalizedMessagelists so the invariant covers field-level errors too.Result<T>.Okrejects null;Result<Unit>is the payload-less success shape.Result.FailFor<TResponse>returns the concreteTResponsevia reflection so MediatR pipeline behaviors short-circuit correctly.Entity<TId>+AuditableEntity<TId>aggregate bases with transient + cross-runtime-type equality guards (EF change-tracker safe), the audit-column / soft-delete / optimistic-concurrency contract, andMarkCreatedthrowing on second call.UserIdVogen value object in SharedKernel.Identifiers — audit actor columns are strongly typed before the Identity module ships.CursorPagination/Page<T>/PageInfo) matching Standards 04 § Pagination; ctor validatesLimit > 0(kernel-level guard).IDomainEvent : INotification+DomainEventbase withrequired initEventId/OccurredAtso events always stamp through the injectedIGuidFactory/IClock.LearnStackVogenDefaults.IdMask; the test project carries a syntheticTestIdend-to-end smoke test of the emitter pipeline.Corpus updates
phase-02a-kernel-tenancy.md— Packet 2 marked done with review fold-in record.Standards 01 § Dependency Direction— new "Build-time-only exceptions" sub-section sanctioning the EF Core (Vogen-emitted converter) and MediatR (INotificationmarker) references SharedKernel + everyModules.<X>.Domainrequires.Standards 02+09— Result/Error code snippets reflect the refactored shape.ADR-0023— Amendment 1 records the 6.x → 7.0.0 version drift and the architecture-test deferral; Implementation Notes carry the literal pin +UserIdadded to the cross-cutting VO list.architecture/15— outbox exampleLocalizedMessage.Of(...)now honours the prefix invariant.glossary.md— entries forLocalizedMessage,Unit,UserId,IClock/IRandom/IGuidFactory,Entity<TId>/AuditableEntity<TId>,IDomainEvent,CursorPagination/Page<T>/PageInfo,LearnStackVogenDefaults.IdMask; refreshedResult<T>andError.Commits
db230aefeat — land Packet 2 surface377b34bdocs — ADR-0023 Amendment 1 + architecture/15 lockey_ honouring7c9133afix — review blocker + major findings (Entity equality, FailFor shape, Ok null guard, UserId, DomainEvent required init, CursorPagination ctor, …)a1ad5fbdocs — corpus sync for review-driven decisions (Standards 01 build-time exceptions, 02/09 snippet refresh, glossary, ADR-0023 literal pin)Test plan
dotnet build /p:CI=true(TreatWarningsAsErrors) — 0 warnings, 0 errorsdotnet test tests/LearnStack.Tests.Unit— 64/64 green (includes new transient + cross-type equality tests, FailFor<Result>, Ok null guard, MarkCreated second-call throw, CursorPagination ctor validation, LocalizedMessage defensive copy + structural equality)dotnet test tests/LearnStack.Tests.Architecture— 17/17 greendotnet test tests/LearnStack.Tests.Contract— 1/1 greendocs/analysis/residual scan over changed files — clean🤖 Generated with Claude Code
Summary by Sourcery
Introduce shared kernel primitives for time, randomness, identifiers, domain entities, results, pagination, and domain events, and align documentation and tests with the new core contracts.
New Features:
Enhancements:
Build:
Documentation:
Tests:
Summary by CodeRabbit
New Features
Documentation